Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In debug, avoid force casting if an Autoload object is freed. #92608

Closed

Conversation

cyanglaz
Copy link

When checking an autoload object, they are force casted, which might result a crash if the object is freed.

This PR adds an additional check to prevent the force casting.

Fixes #92607

@cyanglaz cyanglaz requested a review from a team as a code owner May 31, 2024 21:43
@@ -403,6 +403,11 @@ void GDScriptLanguage::debug_get_globals(List<String> *p_globals, List<Variant>

const Variant &var = gl_array[E.value];
if (Object *obj = var) {
bool freed = false;
var.get_validated_object_with_check(freed);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can create another method in Variant to just check the validity?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed as you'd always just be fetching the object, and not needed for this case see below

@@ -403,6 +403,11 @@ void GDScriptLanguage::debug_get_globals(List<String> *p_globals, List<Variant>

const Variant &var = gl_array[E.value];
if (Object *obj = var) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (Object *obj = var) {
bool freed = false;
if (Object *obj = var.get_validated_object_with_check(freed)) {

Instead, no need to complicate things

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@cyanglaz cyanglaz force-pushed the fix_debugger_crash_invalid_object branch from 36fdecb to 0e4d0a2 Compare June 1, 2024 12:40
@cyanglaz cyanglaz requested a review from AThousandShips June 1, 2024 12:40
@cyanglaz
Copy link
Author

cyanglaz commented Jun 6, 2024

@AThousandShips friendly ping, I have updated the code per your request, PTAL :)

Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to squash the commits and write a proper commit message, but the change itself looks good to me.

@vnen
Copy link
Member

vnen commented Jul 25, 2024

I just realized there is a previous PR doing the same thing: #89274

@akien-mga
Copy link
Member

Superseded by #89274. Thanks for the contribution!

@akien-mga akien-mga closed this Jul 26, 2024
@akien-mga akien-mga removed this from the 4.3 milestone Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Freed autoload object causes debugger to crash without a meaningful error message (GDScript)
5 participants